-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix aggregate cursor als #15094
base: master
Are you sure you want to change the base?
Fix aggregate cursor als #15094
Conversation
46fc7ed
to
51e6b08
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a couple of minor style suggestions, but looks good overall.
lib/aggregate.js
Outdated
*/ | ||
|
||
Aggregate.prototype._optionsForExec = function() { | ||
const options = clone(this.options || {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clone()
shouldn't be necessary here, would be ideal to avoid cloning for performance purposes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous implementation also used clone(), so I retained it to ensure no existing functionality was unintentionally affected. However, I agree that removing clone() could improve performance. If it’s deemed unnecessary, I suggest calling the function directly in the constructor to initialize the options instead.
lib/cursor/aggregationCursor.js
Outdated
@@ -57,21 +58,25 @@ util.inherits(AggregationCursor, Readable); | |||
function _init(model, c, agg) { | |||
if (!model.collection.buffer) { | |||
model.hooks.execPre('aggregate', agg, function() { | |||
Object.assign(c.options, agg._optionsForExec()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to previous comments, is there any need to set c.options
at all? Could just call aggregate(agg._pipeline, agg.optionsForExec())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In QueryCursor, options is also set to a new object, so I followed the same approach here.
That said, QueryCursor uses the options object for some checks while AggregateCursor only uses it in _init, so it might make sense to remove it from AggregateCursor for now.
Removed the cloning in _optionsForExec and also sest the options before creating AggregationCursor in Aggregate.cursor(). Passed all tests with npm test. |
…s in AggregationCursor.
6ec8cd5
to
4b8a25e
Compare
Summary
Aggregate cursors didn't automatically add session if it was present in AsyncLocalStorage. Also the options weren't the same for exec and cursor so I added a function to make them the same just like in QueryCursor.
Examples